pkg/remote/conncontroller/conncontroller.go (1)

Line range hint 191-204: Avoid logging sensitive information such as domain socket names

Logging the generated domain socket name may expose sensitive information in logs. Consider removing the socket name from the log messages or obfuscating it to prevent potential security risks.

Apply this diff to remove the socket name from the log messages:

-	conn.Infof(ctx, "generated domain socket name %s\n", sockName)
+	conn.Infof(ctx, "domain socket listener created\n")

pkg/wshutil/wshrpcio.go (1)

65-78: Consider buffering the channel for better performance.

The implementation is correct, but unbuffered channels can lead to potential blocking in high-throughput scenarios.

-	ch := make(chan LineOutput)
+	ch := make(chan LineOutput, 1024) // Buffer size can be adjusted based on expected load

cmd/wsh/cmd/wshcmd-conn.go (1)

180-184: Inconsistent structure usage between functions.

While other functions use ConnExtData, this function uses ConnRequest. Consider standardizing the structure usage across similar functions for better maintainability.

-	data := wshrpc.ConnRequest{
-		Host:       connName,
-		LogBlockId: RpcContext.BlockId,
-	}
+	data := wshrpc.ConnExtData{
+		ConnName:   connName,
+		LogBlockId: RpcContext.BlockId,
+	}

pkg/wsl/wsl.go (1)

449-451: Consider a more descriptive variable name.

The implementation correctly uses the config watcher and improves default value handling. However, the variable name wshAsk could be more descriptive.

Apply this diff for better clarity:

-wshAsk := wconfig.DefaultBoolPtr(config.Settings.ConnAskBeforeWshInstall, true)
+shouldPromptForWshInstall := wconfig.DefaultBoolPtr(config.Settings.ConnAskBeforeWshInstall, true)
-installErr := conn.CheckAndInstallWsh(ctx, conn.GetName(), &WshInstallOpts{NoUserPrompt: !wshAsk})
+installErr := conn.CheckAndInstallWsh(ctx, conn.GetName(), &WshInstallOpts{NoUserPrompt: !shouldPromptForWshInstall})

frontend/app/store/wshclientapi.ts (1)

41-41: Add JSDoc comments for the modified and new methods.

The implementation correctly updates the parameter types and adds the new method. Consider adding JSDoc comments to document the parameters and return types.

Add documentation like this:

/**
 * Ensures a connection is established with the specified connection data.
 * @param client The WebSocket client instance
 * @param data The connection extension data
 * @param opts Optional RPC options
 * @returns A promise that resolves when the connection is ensured
 */
ConnEnsureCommand(client: WshClient, data: ConnExtData, opts?: RpcOpts): Promise<void>

/**
 * Reinstalls the Wave Shell Extensions for a connection.
 * @param client The WebSocket client instance
 * @param data The connection extension data
 * @param opts Optional RPC options
 * @returns A promise that resolves when the reinstallation is complete
 */
ConnReinstallWshCommand(client: WshClient, data: ConnExtData, opts?: RpcOpts): Promise<void>

/**
 * Appends output data to a command controller.
 * @param client The WebSocket client instance
 * @param data The output data to append
 * @param opts Optional RPC options
 * @returns A promise that resolves when the output is appended
 */
ControllerAppendOutputCommand(client: WshClient, data: CommandControllerAppendOutputData, opts?: RpcOpts): Promise<void>

Also applies to: 51-51, 60-64

pkg/wshrpc/wshserver/wshserver.go (3)

265-276: Add input validation for empty or malformed data.

Consider validating the input data before processing to ensure robustness:

• Check if data.Data64 is empty
• Validate if data.BlockId is empty

 func (ws *WshServer) ControllerAppendOutputCommand(ctx context.Context, data wshrpc.CommandControllerAppendOutputData) error {
+    if data.BlockId == "" {
+        return fmt.Errorf("blockId cannot be empty")
+    }
+    if data.Data64 == "" {
+        return fmt.Errorf("data64 cannot be empty")
+    }
     outputBuf := make([]byte, base64.StdEncoding.DecodedLen(len(data.Data64)))
     nw, err := base64.StdEncoding.Decode(outputBuf, []byte(data.Data64))

---

614-627: Improve error handling for block retrieval.

The error from DBMustGet is silently ignored. Consider logging the error or propagating it to help with debugging.

 func termCtxWithLogBlockId(ctx context.Context, logBlockId string) context.Context {
     if logBlockId == "" {
         return ctx
     }
     block, err := wstore.DBMustGet[*waveobj.Block](ctx, logBlockId)
     if err != nil {
+        log.Printf("error getting block for logging context: %v", err)
         return ctx
     }

---

629-635: Maintain consistent error message formatting.

The error messages in connection-related functions should follow a consistent format. Consider using a helper function to format connection-related errors.

+func formatConnError(connName string, err error) error {
+    return fmt.Errorf("connection error for %q: %v", connName, err)
+}

 func (ws *WshServer) ConnEnsureCommand(ctx context.Context, data wshrpc.ConnExtData) error {
     ctx = termCtxWithLogBlockId(ctx, data.LogBlockId)
     if strings.HasPrefix(data.ConnName, "wsl://") {
         distroName := strings.TrimPrefix(data.ConnName, "wsl://")
-        return wsl.EnsureConnection(ctx, distroName)
+        if err := wsl.EnsureConnection(ctx, distroName); err != nil {
+            return formatConnError(data.ConnName, err)
+        }
+        return nil
     }
-    return conncontroller.EnsureConnection(ctx, data.ConnName)
+    if err := conncontroller.EnsureConnection(ctx, data.ConnName); err != nil {
+        return formatConnError(data.ConnName, err)
+    }
+    return nil
 }

Also applies to: 680-699

frontend/types/gotypes.d.ts (1)

128-132: Add JSDoc comments for new type declarations.

Consider adding documentation for the new types to improve code maintainability:

+/**
+ * Data structure for appending output to a block file.
+ */
 type CommandControllerAppendOutputData = {
     blockid: string;
     data64: string;
 };

+/**
+ * Extended connection data with logging context.
+ */
 type ConnExtData = {
     connname: string;
     logblockid?: string;
 };

Also applies to: 295-299

frontend/app/block/blockframe.tsx (2)

359-363: Consistent timeout handling for connection operations.

Consider extracting the timeout value to a constant to maintain consistency across all connection operations.

+const CONN_OPERATION_TIMEOUT = 60000;

 const handleTryReconnect = React.useCallback(() => {
     const prtn = RpcApi.ConnConnectCommand(
         TabRpcClient,
         { host: connName, logblockid: nodeModel.blockId },
-        { timeout: 60000 }
+        { timeout: CONN_OPERATION_TIMEOUT }
     );

---

548-552: Consistent error handling for connection operations.

The error handling for connection operations should be consistent. Consider extracting the error handling logic to a reusable function.

+const handleConnError = (operation: string, blockId: string, connName: string, error: any) => {
+    console.log(`error ${operation}`, blockId, connName, error);
+};

 RpcApi.ConnEnsureCommand(
     TabRpcClient,
     { connname: connName, logblockid: nodeModel.blockId },
     { timeout: 60000 }
-).catch((e) => {
-    console.log("error ensuring connection", nodeModel.blockId, connName, e);
-});
+).catch((e) => handleConnError("ensuring", nodeModel.blockId, connName, e));

Also applies to: 702-706

pkg/remote/sshclient.go (1)

156-156: Comprehensive logging implementation.

The addition of debug logging provides better visibility into the SSH connection process. Consider adding log levels to control verbosity.

+const (
+    LogLevelDebug = "debug"
+    LogLevelInfo  = "info"
+)

+func logWithLevel(ctx context.Context, level string, format string, args ...interface{}) {
+    if level == LogLevelDebug {
+        blocklogger.Infof(ctx, "[conndebug] "+format, args...)
+    }
+}

Also applies to: 223-223, 238-238, 241-241, 640-640, 643-643, 647-647, 650-650, 656-656, 659-659

frontend/app/view/term/term.tsx (1)

685-723: LGTM! Consider adding documentation for the debug levels.

The implementation of the "Debug Connection" menu item is clean and follows the existing patterns. The debug levels (Off, Info, Verbose) are well-structured with clear metadata values.

Consider adding code comments or documentation to explain:

• The purpose of each debug level
• The expected logging output for each level
• The impact on performance when debugging is enabled

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro